-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Add additional physical read/write metrics to oracledb receiver #37814
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add additional physical read/write metrics to oracledb receiver #37814
Conversation
Looks good, please attend to my comment. Please add a changelog. |
@atoulme |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Would you please run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@@ -109,6 +109,51 @@ metrics: | |||
value_type: int | |||
input_type: string | |||
unit: "{reads}" | |||
oracledb.physical_reads_direct: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically the format and names of these metrics should match semantic conventions, but we're in a tough spot given existing metrics don't match. (We either match this component's existing metrics, or end up matching semantic conventions and make this component's metrics inconsistent.)
One example that I'm referring to, the semantic convention is to have read and write metrics under the same metric name, with a "direction" attribute that is used to show if the data point is for reads or writes.
I think for now it's okay to add these metrics the way this PR proposes, but we'll likely need a breaking change in the future to fix all of them together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely something to do at another time for all the metrics in one fell swoop (and as a breaking change), if that's what we want.
One advantage of this deviation from the semantic conventions here though is that the metric names match the names from the Oracle documentation. This may help users enable/disable the metrics they care about since they also are OracleDB users if they use this receiver.
@ccressent moving to draft so you can look into the feedback from codeowners, please mark as ready for review afterwards. |
Co-authored-by: Curtis Robert <[email protected]>
0a9bccb
to
6368c4a
Compare
Please try and run |
@crobert-1 please review one more time and mark ready to merge if it looks good to you. |
…-telemetry#37814) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description This PR adds the following metrics to the `oracledb` receiver: - 'physical reads direct' (disabled by default) - 'physical read io requests' (disabled by default) - 'physical writes' (enabled by default) - 'physical writes direct' (disabled by default) - 'physical write io requests' (disabled by default) <!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. --> #### Link to tracking issue N/A <!--Describe what testing was performed and which tests were added.--> #### Testing Manual testing. <!--Describe the documentation added.--> #### Documentation Automatically generated by `mdatagen` for each metric. --------- Co-authored-by: Antoine Toulme <[email protected]> Co-authored-by: Curtis Robert <[email protected]>
Description
This PR adds the following metrics to the
oracledb
receiver:Link to tracking issue
N/A
Testing
Manual testing.
Documentation
Automatically generated by
mdatagen
for each metric.